-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: deny unused results #1825
chore: deny unused results #1825
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1825 +/- ##
===========================================
- Coverage 86.51% 86.22% -0.30%
===========================================
Files 587 587
Lines 95642 95607 -35
===========================================
- Hits 82745 82434 -311
- Misses 12897 13173 +276 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lint is too verbose. Could we add #[must_use]
to methods that need this check instead?
https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute
I still prefer using unwrap() as it yields the error message. Assertion on |
e2fb357
to
db6edab
Compare
db6edab
to
4e9a76f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* chore: deny unused results * rebase
I hereby agree to the terms of the GreptimeDB CLA
What's changed and what's your intention?
The rustc lint: "deny unused-results" can check whether a return value is handled or not.
Pros to this lint: force people to deal with all return values, reduce the possibility of lurked bugs. Cons: sometimes could be annoying, even the most trivial return values (
HashMap::insert
for example) have to be dealt with.According to our internal poll to date (positive 4, neutral 4, negative 0), we decide to enable it.
This PR goes through all our codes and fix all the warnings. Mainly:
let _ = ...
HashMap::from
where possibleassert!(result.is_ok())
instead ofresult.unwrap()
Checklist
Refer to a related PR or issue link (optional)